Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Exceptions] - Exceptions modal pt 2 #70886

Merged
merged 6 commits into from
Jul 10, 2020

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Jul 6, 2020

Summary

Part 2 of the exceptions modal feature. Adds on more functionality and tests to the exceptions modal, and switches certain patterns from a testing convention to what they will be in actual usage.

Screen Shot 2020-07-09 at 12 29 00 PM

Screen Shot 2020-07-09 at 4 59 41 PM

  • Endpoint exceptions OS is always enriched with windows. It doesn’t use alert data.
  • Alerts are generated when the…. text should also appear when editing endpoint exceptions
  • When bulk close checkbox is disabled, value should be set to false. I think you can get into a state where the checkbox is disabled but shouldBulkClose is true
  • Don’t refresh viewer when the user clicks Cancel in the modal
  • Use the correct index for endpoint exception autosuggest
  • Look into moving the entire enrichExceptionItems function to the helpers file. Use it in both edit and add modal.
  • ^I think we've already abstracted out the parts of this function that we can, the rest are unique to their individual use cases, they only share their title
  • Add jsdoc comments above to helper function exports
  • In getOperatingSystems helper, OS should change to macOS. Try to consolidate with the other OS helper
  • Remove entryHasListType [Security Solution][Exceptions] - Exception Modal Part I #70639 (comment)
  • ^ combined those two helpers into this function because the other one wasnt being used anywhere
  • Look into improving getMappedNonEcsValue helper [Security Solution][Exceptions] - Exception Modal Part I #70639 (comment)
  • Disable "Add Endpoint Exception" option for non-Endpoint alerts
  • Add tests for helper functions

Coming in Part 3

  • In add exception modal, if the close checkbox is selected, refresh the alerts table on submit
  • Look into using lists plugin hooks like the persistExceptionItem hook
  • Filter out fields not included in the list of Endpoint "exceptionable" fields when the user tries to add an Endpoint Exception
  • Allow the user to add Exceptions from the Timeline
  • Disable ORs when editing an exception item. Need to coordinate with @dontcallmesherryli
  • Add tests for react components
  • Handle case when user deletes an item in the Edit Modal.
  • Implement bulk close. Currently the checkbox doesn't trigger a bulk close action on the server.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dontcallmesherryli
Copy link

dontcallmesherryli commented Jul 8, 2020

Some things I observed that needs to be addressed that isn't in checklist above:

  1. "Open alerts", "In progress alerts", and "Closed alerts" should loose the "alert" wording. --> Tabs named "Open", "In progress" and "Closed".
    image
  2. According to Marra's mocks, I thought the action overflow menu would also include "edit action" as option.
    Mocks for Exception: https://www.figma.com/file/jcCKnGXvOlFxMOpUjlTMMz/All-Exceptions?node-id=1%3A2047
    image
  3. This is in the checklist: Disable "Add Endpoint Exception" option for non-Endpoint alerts. -- Please make sure to disable it both in the Alert List overflow menu, as well as in the rules exception page:
    image
  4. The spacing for Endpoint exception modal seems off. The "close alert" part on the bottom seems to be taking too much real estate.
    image
  5. Weird loading of the action overflow menu when user clicks on overflow menu before the page fully loads. It's just texts of the action options floating on top of the alert list.
  6. I can add an Endpoint exception without any info entered in the current Endpoint exception modal. When viewing the exception in the rules page, it shows a much of empty fields. (Is that expected behavior?)
    image (45)
  7. Within the Alert list, once user selects "select all 1,700 alerts", this selection doesn't auto unselect when user finishes taking action on the full selection. So when user wants to select 1 or 2 alerts and do things to it, it's still selecting 1,700 alerts. See screenshot:
    image

@dplumlee dplumlee changed the title Exception modal 2 [Security Solution][Exceptions] - Exceptions modal pt 2 Jul 9, 2020
@dplumlee dplumlee marked this pull request as ready for review July 9, 2020 15:17
@dplumlee dplumlee requested review from a team as code owners July 9, 2020 15:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)


function doesFieldNameExist(exceptionEntry: Entry) {
return indexPatterns.fields.find(({ name }) => name === exceptionEntry.field) === undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this named right? It looks like it seeing if there is nothing here through the check against undefined? As if it is saying "doesFieldNameNotExist"?

Some pointers/nits (nothing to hold this up), I would still suggest we follow the flow of most of the code base looking at == null rather than something as specific as undefined since == null will catch both null and undefined values. Most of the other plugins and core code base typically follow that pattern.

I would also use a .some vs. a .find() === undefined if that makes more semantic sense within arrays. Most of the times these days I rarely find a reason to use a for loop instead of one of the more semantic array methods such as .some, .every, .map, .reduce, .flat. etc.... I still sometimes need to use a .forEach or for of but I almost always use it as a last resort and try to see if there's a more semantic loop from the array that makes sense first.

Even looking above this at the loops above it, I mentally rewrite it in my mind as a series of .some() loops as it seems to be doing a series of loops looking for the first entry to become true to return true but if it cannot find it, then it returns false.

For pure functions, I would be careful of introducing functions within functions. They're harder to isolate and test vs pulling the function out and then testing outside of the original function. Since they're pure functions, I don't really worry about other people calling them if they want to as it doesn't have side effects.

Although we don't have strict linter rules around ES2015+ style fat arrows vs. function names, I would stick to one style within a file when you can and use for example the ES2015+ style.

I still recommend to define the return types within TypeScript for functions as it makes it easier to keep other maintainers from accidentally introducing a new return type by accident.

@@ -65,7 +66,7 @@ interface AddExceptionModalProps {
nonEcsData: TimelineNonEcsData[];
};
onCancel: () => void;
onConfirm: () => void;
onConfirm: (didCloseAlert?: boolean) => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems from all the uses of onConfirm that the boolean argument is always present, do we need the ??

const enrichExceptionItems = useCallback(() => {
let enriched: Array<ExceptionListItemSchema | CreateExceptionListItemSchema> = [];
enriched =
comment !== ''
? enrichExceptionItemsWithComments(exceptionItemsToAdd, [{ comment }])
: exceptionItemsToAdd;
if (exceptionListType === 'endpoint') {
const osTypes = alertData ? ['windows'] : ['windows', 'macos', 'linux'];
const osTypes = alertData ? retrieveAlertOsTypes() : ['windows', 'macos', 'linux'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Might be able to just clean up a bit and add the alertData check within retrieveAlertOsTypes so if someone else later uses it they don't need to remember to make this check?

@@ -248,6 +258,36 @@ describe('Exception helpers', () => {
});
});

describe('#getEntryValue', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these!

@@ -278,27 +318,33 @@ describe('Exception helpers', () => {

describe('#getOperatingSystems', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be formatOperatingSystems?

const item = data.find((d) => d.field === fieldName);
if (item != null && item.value != null) {
return item.value;
}
return undefined;
return [];
};

export const entryHasListType = (
exceptionItems: Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
) => {
for (const { entries } of exceptionItems) {
for (const exceptionEntry of entries ?? []) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entries gets defaulted to empty array, does Typescript show an error without the ?? [] because then I'm wondering if we missed something somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we type CreateExceptionListItemSchema as a potential type within the array, it's doesn't hasn't been defaulted yet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha. Is this being used to check exceptions created within the builder? If that's the case I would type it to ExceptionsBuilderExceptionItem[]. That uses ExceptionListItemBuilderSchema | CreateExceptionListItemBuilderSchema and CreateExceptionListItemBuilderSchema is

xport type CreateExceptionListItemBuilderSchema = Omit<
  CreateExceptionListItemSchema,
  'meta' | 'entries'
> & {
  meta: { temporaryUuid: string };
  entries: BuilderEntry[];
};

But I guess, that's only if it's being used in the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, only being used in both versions of the exception modals so it's usually being passed that exceptionItemsToAdd we define with the same Array<ExceptionListItemSchema | CreateExceptionListItemSchema> type in component state

@@ -323,20 +338,20 @@ export const getAlertActions = ({
}
},
id: 'addEndpointException',
isActionDisabled: () => !canUserCRUD || !hasIndexWrite,
isActionDisabled: () => !canUserCRUD || !hasIndexWrite || isEndpointAlert() === false,
Copy link
Contributor

@yctercero yctercero Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: I see sometimes we check for falsiness using ! and other times we do === false, is there a specific reason for one over the other throughout? Same with checking for true. I think I've noticed that in the codebase when wanting to check if something exists we use != null to check for both null or undefined. That way when I see someone use ! I usually assume that value is a boolean.

closeAddExceptionModal();
}, [closeAddExceptionModal]);
const onAddExceptionConfirm = useCallback(
(didCloseAlert?: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I might make this explicit and remove the ?

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the changes! I chatted with @peluja1012 about the index/autocomplete weirdness. I don't think it's related to any of the code, but maybe it's something we can circle back on.

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for all the great typescript and work on this. This is really clean easy to read code.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@peluja1012 peluja1012 merged commit c1b2665 into elastic:master Jul 10, 2020
peluja1012 added a commit that referenced this pull request Jul 10, 2020
)

* makes comment updates

* adds tests

* adds back non ecs data to timeline

* comments

* fixes jest tests

* fixes typo

Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 10, 2020
…11y-overlay

* 'master' of github.com:elastic/kibana: (33 commits)
  address index templates feedback (elastic#71353)
  Upgrade EUI to v26.3.1 (elastic#70243)
  [build] Creates Linux aarch64 archive (elastic#69165)
  [SIEM][Detection Engine] Fixes skipped tests (elastic#71347)
  [SIEM][Detection Engine][Lists] Adds read_privileges route for lists and list items
  [kbn/optimizer] implement "requiredBundles" property of KP plugins (elastic#70911)
  [Security Solution][Exceptions] - Exceptions modal pt 2 (elastic#70886)
  [ML] DF Analytics:  stop status polling when job stopped (elastic#71159)
  [SIEM][CASE] IBM Resilient Connector (elastic#66385)
  jenkins_xpack_saved_objects_field_metrics.sh expects to be run from the KIBANA_DIR in CI
  Deduplication of entries and items before sending to endpoint (elastic#71297)
  [services/remote/webdriver] fix eslint error (elastic#71346)
  send slack notifications on visual baseline failures
  fix visual regression job (elastic#70999)
  [Ingest Manager] Add schema to usageCollector. (elastic#71219)
  [ftr] use typed chromeOptions object, adding TEST_BROWSER_BINARY_PATH (elastic#71279)
  [Ingest Manager] Fix limited packages incorrect response (elastic#71292)
  Support multiple features declaring same properties (elastic#71106)
  [Security_Solution][Resolver]Add beta badge to Resolver panel (elastic#71183)
  [DOCS] Clarify trial subscription levels (elastic#70636)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 13, 2020
* master: (78 commits)
  Bump lodash package version (elastic#71392)
  refactor: 💡 use allow-list in AppArch codebase (elastic#71400)
  improve bugfix 7198 test stability (elastic#71250)
  [Security Solution][Ingest Manager][Endpoint] Optional ingest manager (elastic#71198)
  [Metrics UI] Round metric threshold time buckets to nearest unit (elastic#71172)
  [Security Solution][Endpoint] Policy creation callback fixes + Improved error handling in user manifest loop (elastic#71269)
  [Security Solution] Allow to configure Event Renderers settings (elastic#69693)
  Fix a11y keyboard overlay (elastic#71214)
  [APM] UI text updates (elastic#71333)
  [Logs UI] Limit `extendDatemath` to valid ranges (elastic#71113)
  [SIEM] fix tooltip of notes (elastic#71342)
  address index templates feedback (elastic#71353)
  Upgrade EUI to v26.3.1 (elastic#70243)
  [build] Creates Linux aarch64 archive (elastic#69165)
  [SIEM][Detection Engine] Fixes skipped tests (elastic#71347)
  [SIEM][Detection Engine][Lists] Adds read_privileges route for lists and list items
  [kbn/optimizer] implement "requiredBundles" property of KP plugins (elastic#70911)
  [Security Solution][Exceptions] - Exceptions modal pt 2 (elastic#70886)
  [ML] DF Analytics:  stop status polling when job stopped (elastic#71159)
  [SIEM][CASE] IBM Resilient Connector (elastic#66385)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants